Skip to content

Add creating extension packs when opening the editor#2300

Merged
koesie10 merged 1 commit intomainfrom
koesie10/create-extension-pack
Apr 14, 2023
Merged

Add creating extension packs when opening the editor#2300
koesie10 merged 1 commit intomainfrom
koesie10/create-extension-pack

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Apr 12, 2023

This adds the ability to create a new extension pack when opening the data extension editor. It lets the user select a workspace folder and package name, but does not currently allow selecting the actual directory where it will be saved. For now, this is always in a subfolder of the workspace folder.

Screen.Recording.2023-04-12.at.14.56.36.mov

Based on #2296

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 requested review from a team as code owners April 12, 2023 12:58
Comment on lines +225 to +228
const packPath = join(workspaceFolder.path, matches.groups.name);
if (await pathExists(packPath)) {
return `A pack already exists at ${packPath}`;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this path is in a folder that already contains a qlpack (or a qlpack is in a parent folder)? I think we need to check for this case and throw an error.

It's technically possible to do this, but it's confusing and there are some edge cases where problems occur.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a simple prototype, I don't think we need to handle this right now. This will almost always create a nested extension pack since the workspace folder is probably codeql-custom-queries-java which is a qlpack. I've added this to a list of problems we need to solve to make this production-ready.

version: "0.0.0",
library: true,
extensionTargets: {
"codeql/java-all": "*",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if someone tries to create an extension pack targeting something besides java?

Either make it explicit in one of the dropdowns that this only works for java, or add a new step where users can choose the language.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, we're only supporting Java in the data extension editor prototype and don't need to support any other languages yet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine as long as the extension editor clearly states that this only works for Java. (Maybe this already exists...I haven't reviewed that part yet.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have that yet, but it will be made clear to the users we're releasing this to and we'll be adding C# support before doing that anyway, so then this will change to codeql/${databaseItem.language}-all.

@koesie10 koesie10 requested review from aeisenberg and starcke April 13, 2023 08:09
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. See my comment for some sort of disclaimer to add to the extension editor.

version: "0.0.0",
library: true,
extensionTargets: {
"codeql/java-all": "*",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine as long as the extension editor clearly states that this only works for Java. (Maybe this already exists...I haven't reviewed that part yet.)

Base automatically changed from koesie10/create-extension-model-file to main April 14, 2023 09:09
@koesie10 koesie10 merged commit 47d7533 into main Apr 14, 2023
@koesie10 koesie10 deleted the koesie10/create-extension-pack branch April 14, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants